Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-export Event in types.py #2829

Merged
merged 3 commits into from
Mar 18, 2024
Merged

Conversation

szokeasaurusrex
Copy link
Member

@szokeasaurusrex szokeasaurusrex commented Mar 15, 2024

End-users may need to use the Event type for their type hinting to work following the Event type changes. However, we define Event in a private module sentry_sdk._types, which provides no stability guarantees.

Therefore, this PR creates a new public module sentry_sdk.types, where we re-export the Event type, and explicitly make it available as public API via sentry_sdk.types.Event. The new sentry_sdk.types module includes a docstring to inform users that we reserve the right to modify types in minor releases, since we consider types to be a form of documentation (they are not enforced by the Python language), but that we guarantee that we will only remove type definitions in a major release.

sentry_sdk/types.py Outdated Show resolved Hide resolved
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@szokeasaurusrex
Copy link
Member Author

Fixed your comment, @antonpirker.

Also, I know we had discussed maybe changing our internal imports of Event to import from types instead of _types; however, it looks like we would need to update many files manually to make this change.

Perhaps, it would make sense just to change the import in api, since that file is likely the one that external users would interact with the most anyway?

@szokeasaurusrex szokeasaurusrex self-assigned this Mar 18, 2024
@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) March 18, 2024 08:24
@antonpirker
Copy link
Member

Its not as important right now do update our internal imports. We can do this in a later PR, with this PR we have everything in place that the users care about. Baby steps to success :-)

@szokeasaurusrex
Copy link
Member Author

@antonpirker, yes of course, I was planning to make any import changes within a separate PR :)

@szokeasaurusrex szokeasaurusrex merged commit 9dc517b into master Mar 18, 2024
123 of 124 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/public-types branch March 18, 2024 08:44
@mdczaplicki
Copy link

What version is it available in?

@szokeasaurusrex
Copy link
Member Author

@mdczaplicki This PR is not released yet. It should be included in our next release!

@mdczaplicki
Copy link

I see. When do you plan to release?

@szokeasaurusrex
Copy link
Member Author

@mdczaplicki, there is no date set yet for the next release. However, we will probably release a new version including these changes sometime this week.

@mdczaplicki
Copy link

Would you say that 1.42 should be yanked? If we use dict[str, Any] for type hints then mypy is screaming that types are not matching. If we however use Event or Hint from sentry_sdk._types then test suite screams that those cannot be imported.

@szokeasaurusrex
Copy link
Member Author

szokeasaurusrex commented Mar 20, 2024

@mdczaplicki Apologies for the inconvenience. We plan to release a new version including these changes later today.

However, we will not yank 1.42 because the code itself remains backwards compatible. If you are encountering problems with the new type hinting, we would recommend updating your mypy configuration (e.g. you could import Event from sentry_sdk._types with a # type: ignore comment), or ideally updating to this new version once it is released later today.

@szokeasaurusrex szokeasaurusrex mentioned this pull request Mar 20, 2024
@HansAarneLiblik
Copy link

HansAarneLiblik commented Mar 22, 2024

Shouldn't you add the following to the file also? Otherwise non-typechecking environment can't do the import

else:
  Event = dict[str, Any]
  Hint = ...

and would instead need to do it ourselves

# my_project/module/types.py

if TYPE_CHECKING:
  from sentry_sdk.types import Event
else:
  Event = dict[str, Any]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants